Skip to content
This repository has been archived by the owner on Jan 21, 2022. It is now read-only.

feat: provide a singleton Clerk instance as the the default export, a… #8

Merged
merged 2 commits into from
Feb 1, 2021

Conversation

yourtallness
Copy link
Contributor

@yourtallness yourtallness commented Jan 30, 2021

…llow usage of a sub-api directly, make all options configurable in a consistent manner

@colinclerk @SokratisVidros Please read the updated README & examples for a better overview of the new proposed usage.

The intention, based on Colin's proposal, was to have 2 potential variants available for import:

  • @clerk/clerk-sdk-node - default export is a singleton instance, good for 95% of cases
  • @clerk/clerk-sdk-node/instance - default export is the constructor, doesn't instantiate a singleton instance

Note: all other deps are exported by both modules so that you only need to import from one source.

Currently the second "manifest" is hindered by the apparent lack of support for multiple entries in tsdx. We'll have to figure out a workaround.

For the time being I'm also exporting the constructor in the first package, so that the developer can instantiate themselves using that.

P.S. The case or Clerk.default is a non-issue after all, caused by the import of an ESM module in a CJS context.
See usage with CJS in the README for more.

…llow usage of a sub-api directly, make all options configurable in a consistent manner
Copy link
Member

@colinclerk colinclerk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Interesting issue with tsdx and multiple manifests. The dx you ended up with still accomplishes the primary goals, though. Thanks!

Copy link
Contributor

@SokratisVidros SokratisVidros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very meticulous! Well done @yourtallness 🥇

2. ENV var (if applicable)
3. default

#### httpOptions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯


The http client used by the sdk is [got](https://github.com/sindresorhus/got).

All resource operations are mounted as sub-APIs on a `Clerk` object and return promises that either resolve with their expected resource types or reject with the error types described below.
All resource operations are mounted as sub-APIs on a `Clerk` class and return promises that either resolve with their expected resource types or reject with the error types described below.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Do we coin the term sub-APIs? I like it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it is kind of deceiving.
It's not like we have a main api and sub-apis.
Trying to refer to a grouping of api operations for a particular resource.
Naming suggestions most welcome.

```

The middleware will set the Clerk session on the request object as `req.session` and simply call the next firmware.
The middleware will set the Clerk session on the request object as `req.session` and simply call the next middleware.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤣 Clerk in IOT!

Copy link
Contributor Author

@yourtallness yourtallness Jan 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

malware 😝

@@ -1,9 +1,9 @@
const { requireSession } = require('@clerk/clerk-sdk-node');
import clerk, { requireSession } from '@clerk/clerk-sdk-node';

function handler(req, res) {
console.log('Session required');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yourtallness Can we fix or surpress these Github actions linting suggestions? They kind of get in the way during the PR review

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#truedat, will look into it

src/index.ts Outdated Show resolved Hide resolved
@yourtallness yourtallness merged commit b4994c8 into main Feb 1, 2021
@yourtallness yourtallness deleted the expose_default_instance branch February 1, 2021 15:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants